-
Notifications
You must be signed in to change notification settings - Fork 44
Switch containers/[image,storage,common] to go.podman.io. #353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughMigration of a module dependency from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @jankaluza. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(3 hunks)pkg/config/config.go(1 hunks)
🔇 Additional comments (5)
go.mod (5)
22-22: Good: added go.podman.io/commonThis aligns with the monorepo migration. Consider pinning to a tagged release if/when available for reproducibility (pseudo-versions are fine short-term).
20-20: Testify upgrade looks fineNo concerns; incremental test-only dependency bump.
72-72: x/net bump observedIncidental to the migration; low risk but keep an eye on any net/http2 behavior changes in CI.
37-37: Confirm no direct legacygithub.com/containers/...imports remain. Thergsearch produced no matches; please manually verify that none are hidden in vendored or generated files.
16-16: Confirmed direct import of gopsutil—no change needed
The packagegithub.com/shirou/gopsutil/v4/processis imported inpkg/process/process.go(line 27), so the direct require ingo.modis valid and should remain.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
I’ve redone the commit against latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/virtio.go (1)
455-484: Clarify the “old vs new syntax” comment in VirtioNet.ToCmdLine (currently reads inconsistent/unfinished).
The logic looks fine, but the comment is hard to trust as written (and appears to miss a closing parenthesis).Proposed comment fix
- if dev.VfkitMagic { - // use the old syntax for backward compatibility (newer pkg/config code generating the command line for a vfkit release without support for the new syntax - // https://github.com/containers/podman/issues/27873 + if dev.VfkitMagic { + // Use the legacy `unixSocketPath=` option when vfkit "magic" is enabled, to stay compatible + // with vfkit builds that don't support the newer `type=unixgram,path=...` syntax. + // https://github.com/containers/podman/issues/27873 fmt.Fprintf(&builder, ",unixSocketPath=%s", dev.UnixSocketPath) } else { builder.WriteString(",type=unixgram") fmt.Fprintf(&builder, ",path=%s", dev.UnixSocketPath) builder.WriteString(",vfkitMagic=off") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modpkg/config/config.gopkg/config/virtio.gopkg/config/virtio_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/config.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
📚 Learning: 2025-10-12T15:51:07.560Z
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
Applied to files:
pkg/config/virtio_test.gopkg/config/virtio.go
🔇 Additional comments (3)
pkg/config/virtio_test.go (1)
272-278: Test expectations correctly track the new VirtioNet.ToCmdLine normalization tounixSocketPath=when vfkit magic is on/default.
The updated golden strings still keep coverage for thevfkitMagic=offpath wheretype=unixgram,path=...is emitted.Also applies to: 393-406, 407-416, 417-426, 461-470
pkg/config/virtio.go (1)
479-481: Mac emission change is a clean readability win.
fmt.Fprintf(&builder, ...)keeps the formatting consistent with the rest of the method.go.mod (1)
20-20: Container-libs migration is complete. No lingeringgithub.com/containers/*imports remain, all code usesgo.podman.io/common, andv0.66.0is a valid released version. No vendor metadata inconsistencies to address.
These go packages were migrated to a monorepo, as stated in the https://blog.podman.io/2025/08/migration-to-the-container-libs-monorepo-is-complete/. This commit updates the crc package to use these packages from new locations. It was generated using following commands: ``` $ find . -type f -name '*.go' -exec sed -i -e 's,"github.com/containers/image,"go.podman.io/image,g' {} \; $ find . -type f -name '*.go' -exec sed -i -e 's,"github.com/containers/common,"go.podman.io/common,g' {} \; $ find . -type f -name '*.go' -exec sed -i -e 's,"github.com/containers/storage,"go.podman.io/storage,g' {} \; $ goimports -v -w . $ git checkout vendor/ $ vi go.mod # to change the storage, image and common imports $ go mod tidy $ go mod vendor ``` Signed-off-by: Jan Kaluza <[email protected]>
These go packages were migrated to a monorepo, as stated in the https://blog.podman.io/2025/08/migration-to-the-container-libs-monorepo-is-complete/.
This commit updates the crc package to use these packages from new locations.
It was generated using following commands:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.